Ensure all agent pool parameters are preserved in managed cluster creates #5521
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
| } | ||
| if properties.KubeletConfig != nil { | ||
| agentPool.KubeletConfig = properties.KubeletConfig | ||
| AvailabilityZones: properties.AvailabilityZones, |
There was a problem hiding this comment.
Most of the new fields here aren't possible to set from an AzureManagedMachinePool, so there's a fair bit of no-op here. I was too lazy to audit which ones can actually be set, but keeping everything here makes for one less thing to worry about in case we do add or remove any features from CAPZ.
There was a problem hiding this comment.
ACK, thx for adding the maintenance comments so we're less likely to omit this in the future
| ScaleDownMode: hubPool.Spec.ScaleDownMode, | ||
| ScaleSetEvictionPolicy: hubPool.Spec.ScaleSetEvictionPolicy, | ||
| ScaleSetPriority: hubPool.Spec.ScaleSetPriority, | ||
| SecurityProfile: hubPool.Spec.SecurityProfile, |
There was a problem hiding this comment.
The main alternative to this approach is to use unkeyed struct fields and let the compiler check that everything is defined, but I think I prefer the sanity check of having both field names on one line like this.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5521 +/- ##
==========================================
+ Coverage 52.57% 52.61% +0.04%
==========================================
Files 272 272
Lines 29470 29485 +15
==========================================
+ Hits 15493 15513 +20
+ Misses 13169 13165 -4
+ Partials 808 807 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/cherry-pick release-1.19 This probably doesn't affect release-1.18 |
|
@nojnhuh: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 13cc4df2543b1fe444eaf04662db7393dbd809ba
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nawazkh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@nojnhuh: new pull request created: #5524 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
5 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Because agent pool definitions are required by the AKS API when a managed cluster is created, CAPZ aggregates
ManagedClustersAgentPools and converts them toManagedClusterAgentPoolProfiles. This conversion must contain no loss in fidelity in order for the agent pools defined in the managed cluster creation to match the agent pools defined by AzureManagedMachinePools and AzureASOManagedMachinePools. If there are differences, those will be reconciled after the ASO ManagedCluster is reconciled when the standalone ASO ManagedClustersAgentPools are reconciled, but that update will fail if any of the differing fields are immutable.I also added a linter check to enforce this going forward.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs:
Release note: